Add auth config for scalar manager #280
Conversation
thongdk8
force-pushed
the
feat/add-auth-config-for-scalar-manager
branch
from
346dc38 to
cd2476b
Compare
| # Whether to enable authorization or not for the web application, if enabled the login, user management page will be available | ||
| enabled: false | ||
| # The base URL of the authorization service, default is same as the scalar-manager-api service | ||
| baseUrl: http://localhost:8080 |
There was a problem hiding this comment.
In the case of the API service running in the same pod with Scalar Manager web doesn't have the persistent endpoints enabled then we can config this baseUrl point to another instance of the Scalar Manager API (in other clusters) to do authentication. Note that the JWT public key need to be configured the same in the API instances
| apiVersion: v1 | ||
| kind: Service |
There was a problem hiding this comment.
Add a service for API in case we want to expose the API for cross Scalar Manager authentication
There was a problem hiding this comment.
Sorry, I don't know the details of cross Scalar Manager authentication. If I remember correctly, we planned to add the simple authentication feature as a first step on the Scalar Manager Web side only to avoid anonymous login.
So, could you please elaborate on the details of cross Scalar Manager authentication?
I am not able to judge whether this service resource is necessary or not and configuration is proper or not without the detailed information of cross Scalar Manager authentication.
There was a problem hiding this comment.
Ah, let me explain it a bit more based on the current spec we have.
The cross authentication is in the case: We have two instances of the SM (Scalar Manager) in different clusters for example, one is deployed with a configured database and it has the authentication endpoints, once login the user will have the JWT tokens to access other protected endpoints from that instance (e.g pause, schedule pause). The other instance is deployed without database config but it is configured with the JWT public key sharing the same as instance 1 so this instance is able to verify requests with the JWT token in the header issued by instance 1, basically client (web UI) can login to SM 1 and access to both SM 1 and SM 2, of course the SM 2 API need to be viable to the SM 1.
Please correct me if I miss something @ypeckstadt
I made a PR for the docs update that contains a bit more info about deployment configuration here https://github.com/scalar-labs/docs-internal-orchestration/pull/86
| web: | ||
| image: | ||
| repository: ghcr.io/scalar-labs/scalar-manager-web | ||
| pullPolicy: IfNotPresent | ||
| # Overrides the image tag whose default is the chart appVersion. | ||
| tag: "" | ||
|
|
||
| authorization: |
There was a problem hiding this comment.
In the application.properties, it seems that you use the term authentication as a property name. However, in the values.yaml file, you use authorization.
Is there any reason why you use the term authorization here?
Sorry if I missed something, but I think it would be better to use the same term authentication to avoid unnecessary confusion.
There was a problem hiding this comment.
Actually in the application.properties I also used the authorization in app.authorization.enabled config. The authentication word is used in the prefix for the JWT config only authentication.providers.static-jwt.xxx.
The main reason that authorization makes more sense in our case that the API can be deployed without authentication endpoints but it is still able to verify the request via the JWT in the Authorization header from the requests.
| env: | ||
| - name: NEXT_PUBLIC_AUTH_ENABLED | ||
| value: {{ .Values.web.authorization.enabled | quote }} | ||
| - name: NEXT_PUBLIC_PERSISTENCE_API_BASE_URL | ||
| value: {{ .Values.web.authorization.baseUrl | quote }} | ||
| - name: NEXT_PUBLIC_OPERATION_API_BASE_URL | ||
| value: {{ .Values.web.operation.baseUrl | quote }} |
There was a problem hiding this comment.
Are these environment variables mandatory?
Listing a lot of environment variables here might cause increasing maintenance costs for Helm Chart. This is my concern.
So, I think it would be better to set configurations via application.properties instead of environment variables. In such a case, we might not need to update the Helm Chart even if you add/change some configurations on the Scalar Manager side. It can reduce the maintenance costs in the future.
There was a problem hiding this comment.
These environment variables are mandatory, but they are for the web container. For the api container we still keep the config in the application.properties as it was.
The config for the web container would not be many I think, for now we will have these new three vars.
| apiVersion: v1 | ||
| kind: Service |
There was a problem hiding this comment.
Sorry, I don't know the details of cross Scalar Manager authentication. If I remember correctly, we planned to add the simple authentication feature as a first step on the Scalar Manager Web side only to avoid anonymous login.
So, could you please elaborate on the details of cross Scalar Manager authentication?
I am not able to judge whether this service resource is necessary or not and configuration is proper or not without the detailed information of cross Scalar Manager authentication.
charts/scalar-manager/values.yaml
Outdated
| web: | ||
| type: ClusterIP | ||
| port: 80 | ||
| api: | ||
| type: ClusterIP | ||
| port: 8080 |
There was a problem hiding this comment.
I think it might be more understandable for users to use web.service.*** and api.service.*** instead of service.web.*** and service.api.***.
This is because Scalar Manager Web and Scalar Manager API are deployed as different containers respectively. (I know they are deployed in one pod, but the components are separated in the pod.)
Also, I think it would be better to combine Scalar Manager Web configurations under the web: key and combine Scalar Manager API configurations under the api: key. In this pattern, users can know These configurations are for Web and These configurations are for API clearly.
What do you think?
There was a problem hiding this comment.
Thanks for the suggestion, that makes sense. Let me refactor them.
| type: ClusterIP | ||
| port: 80 |
There was a problem hiding this comment.
I think this update breaks backward compatibility. This is because the existing configuration service.type and service.port will not work after this update.
Is my understanding correct?
Also, if this backward incompatible update is intended, do you expect to release this new update as a new major version release? I want to confirm it, just in case.
There was a problem hiding this comment.
Yes, it will break the backward compatibility as the config will be moved under the web and I think we will make a new major version release. One other way if we want to keep the backward compatibility is keeping the service at the top level as it was for the web and add only a service config for the API under the api config, but that makes the config looks not be in the same style. WDYT?
|
@kota2and3kan Thank you for your feedback, I updated the PR, please take a look again when you have time. |
Description
This PR adds the configurations in the values for configuring the auth feature in the Scalar Manager.
By default, we can still able to deploy the Scalar Manager without auth enabled.
Related issues and/or PRs
NA
Changes made
Checklist
Additional notes (optional)
NA
Release notes
NA